feat(codec): add compression API, standard algorithms, and global reg…#2648
feat(codec): add compression API, standard algorithms, and global reg…#2648sauravzg wants to merge 1 commit into
Conversation
gRPC-Go only implements gzip. Do any other languages offer these other ones out of the box? |
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Please make sure all files include the copyright header.
| use crate::codec::compression::Decompressor; | ||
|
|
||
| /// A read-only interface to query supported compression algorithms. | ||
| pub trait CompressionResolver: Send + Sync + 'static { |
There was a problem hiding this comment.
What problem is solved by putting traits around the registry?
There was a problem hiding this comment.
The traits serve two main purposes:
Testing: Global singletons are painful to test with Rust's parallel test runner. An interface allows for easy mocks or fakes in unit tests.
Interface Segregation/data hiding: Interceptors only need 'get' APIs, so I needed a trait for read-only things. I could've probably achieved this via making the write methods &mut self , but it seems like for global registries interior mutability was more common (tracing lib).
Although, I must admit it's mostly the former, global singletons are painful during testing.
There was a problem hiding this comment.
But this is just a glorified map -- surely we don't need a mock implementation of the map, do we? Whether we have a global instance of it or one that gets passed around is independent from whether we have a trait for it. The trait here feels like overkill.
sauravzg
force-pushed
the
sauravz/compression-api
branch
from
db718f9 to
2e65c81
Compare
…istry This change introduces the foundational compression abstractions and implementations for the grpc server component. - **Compression API (`grpc/src/codec/compression.rs`)**: Defines the `Compressor` and `Decompressor` traits for handling the compression and decompression of gRPC payloads via byte buffers. - **Simplified Design Philosophy**: The API is intentionally kept simple. It deliberately avoids handling concerns like limiting input and output buffer sizes, instead delegating these safety boundaries to the higher-level gRPC business logic. - **Performance Improvements**: By relying on direct buffer manipulation, this implementation avoids the hidden memory copying overhead present in the `tonic` crate (which utilizes 8KB intermediate buffers for I/O). - **Standard Algorithms**: Implements `gzip`, `deflate`, and `zstd` compression logic in their respective modules. These are conditionally compiled based on feature flags. - **Global Registry (`registry.rs`)**: Introduces a thread-safe `GlobalCompressionRegistry` (backed by `RwLock` and `Arc`) to manage registered compressors and decompressors. It handles safe concurrent access and automatically updates the broadcasted `grpc-accept-encoding` headers. - **Module Integration**: Exposes the new `codec` and `compression` modules to the library's root structure.
sauravzg
force-pushed
the
sauravz/compression-api
branch
from
2e65c81 to
c4d2db5
Compare
| use crate::codec::compression::Decompressor; | ||
|
|
||
| /// A read-only interface to query supported compression algorithms. | ||
| pub trait CompressionResolver: Send + Sync + 'static { |
There was a problem hiding this comment.
But this is just a glorified map -- surely we don't need a mock implementation of the map, do we? Whether we have a global instance of it or one that gets passed around is independent from whether we have a trait for it. The trait here feels like overkill.
| /// Returns an `io::Error` if compression fails. Implementations should gracefully | ||
| /// handle constrained `destination` buffers by returning an error rather than panicking | ||
| /// (e.g., by verifying `destination.remaining_mut()` is sufficient before writing). | ||
| fn compress(&self, source: &mut dyn Buf, destination: &mut dyn BufMut) -> Result<(), String>; |
There was a problem hiding this comment.
SendMessage has the return type of Result<Box<dyn Buf + Send + Sync>, String>. I believe these two traits should match (one way or the other). Is there any advantage to having gRPC allocate the destination BufMut as opposed to letting the codec produce the Buf? The latter seems more flexible than the former depending on the implementation of the codec itself -- it seems like if the compression library's API isn't as we expect, then you could end up having to do a byte-wise copy to get it into the BufMut we allocate (put<T:Buf> does a copy).
I believe if we return a Buf that we could have gzip allocate a BytesMut and use that with the gzip library (it impls BufMut) and return it (it also impls Buf).
| /// Returns an `io::Error` if decompression fails. Implementations should gracefully | ||
| /// handle constrained `destination` buffers by returning an error rather than panicking | ||
| /// (e.g., by verifying `destination.remaining_mut()` is sufficient before writing). | ||
| fn decompress(&self, source: &mut dyn Buf, destination: &mut dyn BufMut) -> Result<(), String>; |
There was a problem hiding this comment.
(The same decision would apply here I believe.)
| #[cfg(feature = "zstd")] | ||
| mod zstd; |
There was a problem hiding this comment.
It seems grpc-C++ supports deflate? But I don't think anything supports zstd:
It might even be best to remove deflate for now unless we have an interop test to confirm they work together.
2 participants
…istry
This change introduces the foundational compression abstractions and implementations for the grpc server component.
grpc/src/codec/compression.rs): Defines theCompressorandDecompressortraits for handling the compression and decompression of gRPC payloads via byte buffers.toniccrate (which utilizes 8KB intermediate buffers for I/O).gzip,deflate, andzstdcompression logic in their respective modules. These are conditionally compiled based on feature flags.registry.rs): Introduces a thread-safeGlobalCompressionRegistry(backed byRwLockandArc) to manage registered compressors and decompressors. It handles safe concurrent access and automatically updates the broadcastedgrpc-accept-encodingheaders.codecandcompressionmodules to the library's root structure.